Skip to content

Improve CustomNSError.errorDomain calculation #5132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 5, 2024
Merged

Improve CustomNSError.errorDomain calculation #5132

merged 1 commit into from
Nov 5, 2024

Conversation

ChrisBenua
Copy link
Contributor

Optimized CustomNSError.errorDomain calculation.

String(reflecting:) for Any.Type objects eventually calls _type(:qualified:). But String(reflecting:) call has overhead because of as? checks.

All details are listed in issue.

@ChrisBenua
Copy link
Contributor Author

@swift-ci please test

@parkera
Copy link
Contributor

parkera commented Nov 4, 2024 •
edited
Loading

This patch will help for non-Darwin platforms - the Foundation.framework (Darwin) version is here.

@parkera
Copy link
Contributor

parkera commented Nov 4, 2024

@swift-ci test

parkera
parkera reviewed Nov 4, 2024
View reviewed changes
Sources/Foundation/NSError.swift Outdated
@@ -300,7 +300,7 @@ public protocol CustomNSError : Error {
public extension CustomNSError {
/// Default domain of the error.
static var errorDomain: String {
return String(reflecting: self)
return _typeName(type(of: self), qualified: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmschonfeld you've used this function for Predicate - does it seem right here too?

Copy link
Contributor Author

@ChrisBenua ChrisBenua Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got it little bit wrong. Because it is static var, there is no need to call type(of:), it should be _typeName(self, qualified: true). UPD: fixed it
Screenshot 2024-11-04 at 21 14 25

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah _typeName(self) is a fine way to get the fully qualified name here - just to confirm, @Azoy is it always the case that String(reflecting: someType) will always just return the fully qualified type name here or do you know of any types that might somehow produce a different value via String(reflecting:) here (just to make sure we don't accidentally change behavior)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it should always return a qualified name.

@ChrisBenua
Copy link
Contributor Author

This patch will help for non-Darwin platforms - the Foundation.framework (Darwin) version is here.

Hi Tony!

Thanks for quick review, really appreciate it! Thanks for pointing it out! I'll open same MR shortly.

@ChrisBenua ChrisBenua mentioned this pull request Nov 4, 2024
Merged
@ChrisBenua
Copy link
Contributor Author

Here is the same MR for Darwin platforms:

swiftlang/swift-foundation#1032

@ChrisBenua ChrisBenua force-pushed the ChrisBenua/improve-CustomNSError-domain-calculation branch from 851118b to fe2faba Compare November 4, 2024 18:16
@jmschonfeld
Copy link
Contributor

@swift-ci test

@jmschonfeld
Copy link
Contributor

@swift-ci please test Windows platform

parkera
parkera approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@parkera parkera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing - can we verify we have a unit test for this behavior?

@ChrisBenua
Copy link
Contributor Author

ChrisBenua commented Nov 4, 2024 •
edited
Loading

One more thing - can we verify we have a unit test for this behavior?

Sure, here it is TestNSError.swift

It would be great if you could review similar Pull-Requests:

@jmschonfeld
Copy link
Contributor

It looks like the linux test failed in a SwiftPM test with

Test Case 'PackageCommandTests.testCommandPluginDiagnostics' started at 2024-11-04 20:21:39.518
/home/build-user/swiftpm/Tests/CommandsTests/PackageCommandTests.swift:2433: error: PackageCommandTests.testCommandPluginDiagnostics : XCTAssertTrue failed - unexpected failure matching '' against pattern equal("command plugin: print\n")
Test Case 'PackageCommandTests.testCommandPluginDiagnostics' failed (55.424 seconds)

It's not immediately apparent how that could be related to this change. @ChrisBenua any thoughts, or should we try a re-run to see if it was an intermittent issue

@jmschonfeld
Copy link
Contributor

Windows passed and this isn't platform specific so I'll just rerun Linux and see what happens 🙂

@jmschonfeld
Copy link
Contributor

@swift-ci please test Linux platform

@ChrisBenua
Copy link
Contributor Author

It looks like the linux test failed in a SwiftPM test with

Test Case 'PackageCommandTests.testCommandPluginDiagnostics' started at 2024-11-04 20:21:39.518
/home/build-user/swiftpm/Tests/CommandsTests/PackageCommandTests.swift:2433: error: PackageCommandTests.testCommandPluginDiagnostics : XCTAssertTrue failed - unexpected failure matching '' against pattern equal("command plugin: print\n")
Test Case 'PackageCommandTests.testCommandPluginDiagnostics' failed (55.424 seconds)

It's not immediately apparent how that could be related to this change. @ChrisBenua any thoughts, or should we try a re-run to see if it was an intermittent issue

It's unclear to me how my changes can possibly affect SPM. Linux tests passed after re-run, so most likely it was intermittent issue

@jmschonfeld jmschonfeld merged commit 1acc48b into swiftlang:main Nov 5, 2024
2 checks passed
@ChrisBenua
Copy link
Contributor Author

@jmschonfeld, I was wondering if you could please ask someone from your colleagues to take a look at similar PR: swiftlang/swift#77369?

It would be greatly appreciated. Thank you in advance for your consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Azoy Azoy Azoy left review comments

@parkera parkera parkera approved these changes

@jmschonfeld jmschonfeld jmschonfeld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChrisBenua @parkera @jmschonfeld @Azoy